Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Type-directed optional fields #1023

Closed
wants to merge 1 commit into from
Closed

Type-directed optional fields #1023

wants to merge 1 commit into from

Conversation

friedbrice
Copy link
Contributor

@friedbrice friedbrice commented May 26, 2023 •
edited
Loading

This PR makes a backwards-compatible change to ToJSON and FromJSON that allows the instance to configure the optionality of record fields of that type when using Generic deriving or Template Haskell deriving.

Specifically, we make the following use-facing changes:

class ToJSON a where
    ...
    omitField :: a -> Bool
    omitField _ = False

and

class FromJSON a where
    ...
    omittedField :: Maybe a
    omittedField = Nothing

The new methods have default implementations that preserve the current behavior of Generically-derived and Template Haskell created instances.
Instance authors can use these methods to configure how to represent fields of this type as omitted from a JSON record.
As an added benefit, this PR eliminates the needs for special-case handling of Maybe and Option, significantly simplifying the Generic and Template Haskell code.

This PR solves #646 and #792, and it's similar to #839 by @phadej.
Please let me know what you think, if this PR is a suitable addition to Aeson, and if there's anything about it you'd like me to fix or change.

Thanks much!

friedbrice
friedbrice commented May 26, 2023
View reviewed changes
tests/UnitTests/OptionalFields.hs Outdated
Comment on lines 15 to 18
-- c.f. https://github.com/haskell/aeson/pull/839#issuecomment-782453060
data POC = POC
{ x :: Nullable Int -- Field is required, but can be null.
, y :: Undefineable Int -- Field is optional, but cannot be null.
, z :: NullOrUndefineable Int -- Field is optional, and can be null.
}
Copy link
Contributor Author

@friedbrice friedbrice May 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tests verify that the PR supports your desired behavior, @bergmark :)
(c.f. #839 (comment))

phadej
phadej reviewed May 26, 2023
View reviewed changes
Copy link
Collaborator

@phadej phadej left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Having omitField :: a -> Bool is indeed simpler that what I had in #839.

There probably should be something in ToJSON1 and FromJSON1 classes, as e.g. instance for Fix relies on them. How Fix Maybe should be serialised?

src/Data/Aeson/Types/ToJSON.hs Outdated Show resolved Hide resolved
src/Data/Aeson/Types/ToJSON.hs Outdated Show resolved Hide resolved
@friedbrice
Copy link
Contributor Author

I was able to get the generic classes down to one method like you suggested. I still need to work on the ToJSON1 logic.

@phadej phadej marked this pull request as draft May 26, 2023 21:02
@phadej
Copy link
Collaborator

phadej commented May 26, 2023

I marked this as draft.

Please ping when you seem happy again.

@friedbrice friedbrice force-pushed the master branch 2 times, most recently from 92d0723 to 59c7a73 Compare June 9, 2023 03:47
@friedbrice friedbrice marked this pull request as ready for review June 9, 2023 03:48
@friedbrice friedbrice requested a review from phadej June 9, 2023 03:48
@friedbrice
Copy link
Contributor Author

@phadej I'm pretty happy with it, now. I'm interested in your feedback :-)

@friedbrice
Copy link
Contributor Author

Oh, also, @phadej, I do have some ideas about changes to To/FromJSON1, but I'd like to know more about what you have in mind before attempting that.

phadej
phadej reviewed Jun 9, 2023
View reviewed changes
Copy link
Collaborator

@phadej phadej left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good.

I have further things to do for this, but I think it's easier and fasterfor me to them myself rather than try to explain them.

Could you squash your changes into own commit (and rebase on recent master), so I can continue from there.

parsonsmatt
parsonsmatt reviewed Jun 9, 2023
View reviewed changes
Copy link
Contributor

@parsonsmatt parsonsmatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some doc updates necessary, but this is super cool. ~268 lines of tests for new functionality means that this is really a +145 / -177 LOC change that adds a feature and removes some warts! Well done.

src/Data/Aeson/Types/FromJSON.hs Outdated Show resolved Hide resolved
src/Data/Aeson/Types/ToJSON.hs Outdated Show resolved Hide resolved
tests/Encoders.hs
Comment on lines -300 to -302
--------------------------------------------------------------------------------
-- IncoherentInstancesNeeded
--------------------------------------------------------------------------------
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😄 Briliant

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was like, "whelp, we don't need this anymore"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your help and ideas, btw, @parsonsmatt. The TH was trivially-easy, but the Generics threw me for a loop.

@friedbrice
Copy link
Contributor Author

Could you squash your changes into own commit (and rebase on recent master), so I can continue from there.

Done. Thanks, @phadej !

@phadej
Copy link
Collaborator

phadej commented Jun 9, 2023

@parsonsmatt

Some doc updates necessary,

If you think so please mark or even suggest amendments. I'm mostly happy with docs in this PR.

@friedbrice
Copy link
Contributor Author

friedbrice commented Jun 9, 2023 •
edited
Loading

@phadej there were two places my docs had minor mistakes, but those were taken care of :-)

@friedbrice
Type-directed optional fields
d8a5709 
* Adds 'omitField' to 'ToJSON'

* Adds 'omittedField' to 'FromJSON'
@phadej phadej mentioned this pull request Jun 10, 2023
Merged
@phadej
Copy link
Collaborator

phadej commented Jun 19, 2023

#1039 is merged. Thanks for making the initial version!

@phadej phadej closed this Jun 19, 2023
@phadej phadej mentioned this pull request Jun 19, 2023
Closed
@julmb julmb mentioned this pull request Jun 3, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@parsonsmatt parsonsmatt parsonsmatt left review comments

@phadej phadej phadej left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@friedbrice @phadej @parsonsmatt